Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

"fix" for clone-then-initialize problem #152

Merged
merged 1 commit into from
Jul 16, 2023
Merged

"fix" for clone-then-initialize problem #152

merged 1 commit into from
Jul 16, 2023

Conversation

arjennienhuis
Copy link
Contributor

This PR fixes the problem with clone and initialize.

It adds more locking while PKCS11 is supposed to be thread safe. For me that doesn't feel right. I would deprecate or remove is_initialized():

    #[deprecated]
    pub fn is_initialized(&self) -> bool {
        get_library_info(self).is_ok()
    }

@arjennienhuis arjennienhuis changed the title "fix" for clone-then-initialize problom "fix" for clone-then-initialize problem Jun 17, 2023
@arjennienhuis arjennienhuis mentioned this pull request Jun 17, 2023
Open
@arjennienhuis arjennienhuis force-pushed the fix-initialized-clone branch 2 times, most recently from ea90169 to edea342 Compare June 17, 2023 20:29
ionut-arm
ionut-arm approved these changes Jul 15, 2023
View reviewed changes
Copy link
Member

@ionut-arm ionut-arm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks! 👍🏻

wiktor-k
wiktor-k reviewed Jul 15, 2023
View reviewed changes
Copy link
Collaborator

@wiktor-k wiktor-k left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good 👍

cryptoki/src/context/mod.rs Outdated Show resolved Hide resolved
cryptoki/tests/basic.rs Outdated Show resolved Hide resolved
cryptoki/src/context/mod.rs Outdated
}

/// Check whether the PKCS11 library has been initialized
pub fn is_initialized(&self) -> bool {
self.initialized
*self.initialized.as_ref().read().unwrap()
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Likewise for unwrap/expect.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done.

@arjennienhuis
Add RWLock to initialized
54a799d 
Signed-off-by: Arjen Nienhuis <[email protected]>
@wiktor-k wiktor-k enabled auto-merge July 16, 2023 22:16
wiktor-k
wiktor-k approved these changes Jul 16, 2023
View reviewed changes
Copy link
Collaborator

@wiktor-k wiktor-k left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good now 👌

@wiktor-k wiktor-k merged commit 9b24e61 into parallaxsecond:main Jul 16, 2023
6 checks passed
@arjennienhuis arjennienhuis deleted the fix-initialized-clone branch July 17, 2023 10:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@wiktor-k wiktor-k wiktor-k approved these changes

@ionut-arm ionut-arm ionut-arm approved these changes

@mike-boquard mike-boquard Awaiting requested review from mike-boquard

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@arjennienhuis @wiktor-k @ionut-arm